Skip to content

Add support for arguments with Nil type#160

Merged
attwad merged 5 commits into
attwad:masterfrom
ideoforms:master
Nov 18, 2022
Merged

Add support for arguments with Nil type#160
attwad merged 5 commits into
attwad:masterfrom
ideoforms:master

Conversation

@ideoforms

@ideoforms ideoforms commented Nov 14, 2022

Copy link
Copy Markdown
Contributor

This PR adds support for creating and parsing messages with a Nil argument type. Arguments of Nil type are mapped to a python None (and vice versa).

I have added unit tests for the new functionality, and verified third-party compatibility by sending/receiving messages containing nil parameters with liblo.

I needed this for some features I'm developing for AbletonOSC, in which OSC endpoints need to return lists of arguments which may contain one or more nil values.

Thanks for the great library!

@attwad

attwad commented Nov 15, 2022

Copy link
Copy Markdown
Owner

Thanks for the PR! Any chance you can fix the failing builds on 3.7 and above? Then I'mm merge, thanks.

@ideoforms

Copy link
Copy Markdown
Contributor Author

Done!

@attwad

attwad commented Nov 15, 2022

Copy link
Copy Markdown
Owner

doh, now it seems to be failing only on 3.6...

@ideoforms

Copy link
Copy Markdown
Contributor Author

Ah, in my haste, I removed something that is still required for tests to pass - should be good now 🤞

@attwad

attwad commented Nov 15, 2022

Copy link
Copy Markdown
Owner

sorry for the back and forth but this isn't fixed yet

@ideoforms

Copy link
Copy Markdown
Contributor Author

Apologies, I am a bit baffled - it's bailing at places unrelated to my new additions, stemming from an earlier commit. It's to do with this line in osc_server.py:

    def __init__(self, server_address: Tuple[str, int], dispatcher: Dispatcher, bind_and_activate: bool = True) -> None:  # type: ignore[call-arg]  # https://github.com/python/typeshed/pull/8542

I think it could be fixed by removing warn_unused_ignores = True from setup.cfg, but that doesn't seem entirely satisfactory.
Maybe the author @PeterJCLaw can shed some light on this?

@PeterJCLaw

PeterJCLaw commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

If we're seeing errors due to ignores which were previously needed now being unused (which is what warn_unused_ignores = True enables) then that suggests something has changed in mypy. This is usually a good thing as it means that mypy is now better able to check code (and the ignore is no longer needed).

In this case it could mean that the error code that needs to be ignored has changed. If that's the case and we're happy to keep ignoring whatever is wrong then a valid fix is to update the error code in use. That probably does want to happen as a separate PR though.

This showing up on an unrelated PR is unfortunately a side effect of the linting tools used in CI not being version pinned.

I've put together #161 which should fix the CI, though highlights has an issue on Python 3.6.

@PeterJCLaw

Copy link
Copy Markdown
Contributor

Ok, I think that the typing issues are all sorted now. If you rebase just your original commit onto master I think you should be free of typing errors.

@ideoforms

Copy link
Copy Markdown
Contributor Author

Done — I don't have permissions to run the workflow myself, but hopefully this time we should be good to go. Thanks @PeterJCLaw!

@attwad attwad merged commit 4b40a71 into attwad:master Nov 18, 2022
@attwad

attwad commented Nov 18, 2022

Copy link
Copy Markdown
Owner

All green now, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants